I don't know how @jianjuns and @tnqn will feel about this, but since this is a new API, I think we should follow the K8s convention:

Therefore, we ask that pointers always be used with optional fields that do not have a built-in nil value.

It tends to lead to a lot of pointers, but it makes it clearer what is optional and what is not. When creating a resource with YAML, it makes no difference. When creating a resource in Go, we can use ptr.To from the K8s libraries to make initialization less verbose. What do you think?

Agreed. There maybe a small problem if we upgrade to v1beta1 later to share the same structure with Traceflow though.

I'd rather do the best interface possible now for PacketCapture, and worry about unifying later.

If we have to introduce a new v1beta2 version for Traceflow, it should still be possible to define a conversion webhook to convert between v1beta1 and v1beta2, because of the semantics of the field?

updated. Can you help review this change? I'm not very sure if i have done it right.

I think @antoninbas means:

type UDPHeader struct {
	// SrcPort is the source port.
	SrcPort *int32 `json:"srcPort,omitempty"`
	// DstPort is the destination port.
	DstPort *int32 `json:"dstPort,omitempty"`
}

because they are optional.

Just saw newer comments. Please ignore it.

I'd rather do the best interface possible now for PacketCapture, and worry about unifying later.

If we have to introduce a new v1beta2 version for Traceflow, it should still be possible to define a conversion webhook to convert between v1beta1 and v1beta2, because of the semantics of the field?

it looks like we are missing the srcIP / dstIP properties here?

it's actually not missed, since we have a Source and Destination field in spec, so the srcIP/dstIP is not used. The golang structure still keep these fields in the PR, but i think we can removed them. The whole Packet structure has changed a lot during the review compared to the original one(==Traceflow's Packet strucuture).

Updated this part. Currently the package related structure looks like this:

// Packet includes header info.
type Packet struct {
	IPv6Header      *IPv6Header     `json:"ipv6Header,omitempty"`
	TransportHeader TransportHeader `json:"transportHeader"`
}

Note: remove IPv4Header as it's unused and in another thread, we are discussing if a IPFamily field is needed.

Also in the TransportHeader, we have tcp/icmp/udp strcuture to allow user filter based on transport attributes. For icmp, we don't have any filter yet, so the strucutre is mainly used as a type indicator.

type ICMPEchoRequestHeader struct {
}

cc @jianjuns @tnqn @luolanzone

Can you help review this MR again? Thank you

I will be actively working on this recently.

Is this field mutually exclusive with Namespace / Pod?

not exactly. we don't have this kind of check now. This IP field is used for Pod->IP or IP->Pod filter case.

The only requirement is that at least one pod is specified.

So it would be valid to provide both a destination Pod and a destination IP (which don't match)?

As long as there is one pod present(no matter whether it's in the source or destination), it will work(ignore the ip field in the same endpoint). I didn't add strict validation for the IP related case, not sure what's the best approach.

since it's exclusive , i have added crd validation using oneOf in the yaml. User cannot provide ip and pod both in the source/dest struct in there CR.

@antoninbas found a little problem with this whole omitempty thing.

4ef7a4b

Since i'm using oneOf to perform some validation in the crd, so the related field need to be set to omitempty wether they are pointer or value or not , or the validation will fail. (using yaml is fine, but golang struct is not, eg in tests code)

For example. If i want to use oneOf for source.ip/pod, means only one can be spcified. the following wont work

type Source struct {
	Namespace string `json:"namespace"`
	Pod string `json:"pod"`
	IP string `json:"ip"`
}

spec: crdv1alpha1.PacketCaptureSpec{
					Source: crdv1alpha1.Source{
						Namespace: "test-namespace",
						Pod:       "test-pod",
					},

since there is no omitempty it's actually:

spec: crdv1alpha1.PacketCaptureSpec{
					Source: crdv1alpha1.Source{
						Namespace: "test-namespace",
						Pod:       "test-pod",
                                                IP:         "",
					},

So the oneOf validation will fail. The solution would be change every affected field (including pod) to pointer + omitempty. or keep as value + omitempty. If not set, it won't show up.

Currently i kept the value format and add some of the omitempty tag back, as change to pointer would require some changes on the current codebase. If you still felt this is needed ( > pointer), i can update this later.

well I think that in K8s this would be something like:

type Source {
    SourcePod *PodReference `json:"sourcePod,omitempty"`
    SourceIP *string `json:"sourceIP,omitempty"`
}

type Destination {
    DestinationPod *PodReference `json:"destinationPod,omitempty"`
    DestinationService *ServiceReference `json:"destinationService,omitempty"`
    DestinationIP *string `json:"destinationIP,omitempty"`
}

Technically the pointers are not strictly necessary for the IP fields, but I like the symmetry.

It does make the API more verbose though, cc @tnqn

now that we have addressed the issue, could you put back the oneOfs in the CRD OpenAPI schema?

sure ,already did.

I feel like we should have the option to write the pcap to a local file as well, just for ease of use / testing
Not everyone will want to have an ftp server for this

So do you think keep the files in container is enough? Since this is not a cli, it would be odd to export the files to a location on the host.

If keep it in the container, filepath + container name should be ok?

If keep it in the container, filepath + container name should be ok

Just filepath I think, the container will always be antrea-agent? If users want the file to be on the Node, they can provide a path that is mounted from the host, such as /var/log/antrea.

But there would need to be a way for users to figure out which antrea-agent Pod has the capture file, so that would need to be included in the Status. Are packets captured at the source or the destination or both?

Both could happen, in the source or destination. pod name can be exposed in the status. Just need to think about the value format for the filepath field.

I know I suggested pointers for this originally. Given more recent discussions, I don't really feel strongly about it anymore, so given that 0 is not a valid port number typically, I would be ok with removing the pointers. I'll let @tnqn chime in as well.

temporary reset to int32 type.

My first impression is this should have a pointer because it's optional. if header.SrcPort != nil is the condition in which we should capture specifc source port only, right?

yes, it's optional. sounds more reasonable to set it as pointer.

Is it possible to share some types/constants with Traceflow?

also see discussion #6257 (comment)

for the constants part i think it's unnecessary, each CRD may have more different status in the future.

About types, since Traceflow is in vebeta1 now, are you suggesting we define some common types in neutral place?

I am not sure if we should define common types across versions. Let us see if @tnqn and @antoninbas have an opinion.

I guess in theory we could have common definitions in a neutral directory, it wouldn't need to be under apis/crd/<version>. But if it's not a versioned directory, then we would have to be confident that they are not going to change.

K8s has meta/v1 with comm types, such as Time. I don't know how good of an analogy it is.

Except those meta types in meta/v1. K8s doesn't define common types across versions. If a constant needs to be used by a version, it will just be replicated in that version. I think it's to keep every version's stability and compatability. Otherwise a change in one version would have an impact on other versions.

I am not sure if we should define common types across versions. Let us see if @tnqn and @antoninbas have an opinion.

I think @antoninbas means:

type UDPHeader struct {
	// SrcPort is the source port.
	SrcPort *int32 `json:"srcPort,omitempty"`
	// DstPort is the destination port.
	DstPort *int32 `json:"dstPort,omitempty"`
}

because they are optional.

Except those meta types in meta/v1. K8s doesn't define common types across versions. If a constant needs to be used by a version, it will just be replicated in that version. I think it's to keep every version's stability and compatability. Otherwise a change in one version would have an impact on other versions.

it feels like for packet capture, we don't need to have a fixed list of protocols? we could filter on an arbitrary protocol value
also SCTP is missing

i didn't add support for SCTP yet. will try to work on that. or maybe this is not a must-have feature in this time.

My main point was not about SCTP specifically, but about whether we really need to support a fixed list of protocols, as opposed to supporting a common set of names, but also letting users provide an arbitrary protocol number.

sure. removed this for now and i will double check the impl code and add more tests.

https://github.com/antrea-io/libOpenflow/blob/f8e71bfb83160a95d2ec6ca53bf55e6f821593e9/protocol/ip.go#L17

Not sure if i missed anything here, but the type code defined in this file doesn't have SCTP.

well I think that in K8s this would be something like:

type Source {
    SourcePod *PodReference `json:"sourcePod,omitempty"`
    SourceIP *string `json:"sourceIP,omitempty"`
}

type Destination {
    DestinationPod *PodReference `json:"destinationPod,omitempty"`
    DestinationService *ServiceReference `json:"destinationService,omitempty"`
    DestinationIP *string `json:"destinationIP,omitempty"`
}

Technically the pointers are not strictly necessary for the IP fields, but I like the symmetry.

It does make the API more verbose though, cc @tnqn

What does the file refer to exactly? The file uploaded to fileserver, or the local file in pod?

it's a filename for fileserver and also in local container. After the CR is deleted, local file will be deleted too.

Also @antoninbas suggested maybe add container (in which pod) info to the path so we can also support only save the file in the container thus don't require a fileserver. this may also require some spec changes(make fileserver optional)

What does StartTime represent and which component is responsible for setting it when the source and destination Pods are on two Nodes?

Source. It represents the start time of the capture since we have a timeout settings in the spec, so this field can be used to check whether this capture has been timeout. I will update the comments to make this more clear.

• When the source and destination Pod are on different Node, will it capture packets on both Nodes?
• N means the total of the packets on two Nodes or each?
• What if packets can be captured on source Node and not on destination Node?

No. All sessions will only performing the capture in one node, either in the source or the destination. I think the location doesn't matter so this was hidden from the api spec.

Not sure a single Phase and Reason are enough for all conditions that it can occur when capturing packets on two Nodes and uploading to a server, maybe it will need a list conditions to represent each condition and respective reason and timestamp. But hard to say what are needed until it's implemented, we can defer to the implementation patch.

you mean conditions like Traceflow? Currently i didn't implement this in PacketCapture, not sure if this is needed.

it feels like the default should be "don't filter on protocol", and not ICMP

make sense. updated.

nit: maybe add omitempty

updated.

this doesn't seem correct unless I am missing something. Protocol is not limited to ICMP.

removed this validation rule as it seems unnecessary for now.